-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: per-folder RSS feeds, cont'd #866
base: v4
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few notes. cc @jackyzha0 when you have bandwidth.
quartz.config.ts
Outdated
@@ -70,6 +70,7 @@ const config: QuartzConfig = { | |||
Plugin.ContentIndex({ | |||
enableSiteMap: true, | |||
enableRSS: true, | |||
feedDirectories: ["index"], // For a feed for only pages in content/Folder/, add "Folder" to the array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think enableRSS
should also handle this, need to set custom feedDirectories
.
Are there cases where you don't want to set RSS for folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be duplicative. Enable should toggle the whole state, with optional finer-grained control through a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we should have enableRSS
, and it will do recursive feed for folders, no need to expose this options to users.
For more fine-grained control users can modify the code themselves.
Or at least, we don't show this feedDirectories to quartz.config.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of this PR is to expose RSS generation per-directory for easy configuration without having to modify the code of the plugin. The web is woefully underdeveloped when it comes to RSS and making it more accessible is a big goal of mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem with just generating RSS recursively with enable RSS
?
My point is that if users want more fine grain per directory, they can do that themselves. By default, we just traverse the vault and generate them, right?
Not sure if there is something that I miss here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this is my two cent, probably @jackyzha0 would have better input than me here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, if they set
enable RSS
to false, yet we still generate RSS per folder, wouldn't that be confusing?
ohh, the issue here is a disconnect in what a feed is. some have multiple purposes, but they're only emitted as .xml
RSS feeds if enableRSS
is on. Try the PR with enableRSS: false
in your config. I named the map feedIndices
because they're each a content index which might be a feed (but yeah, love that there are three meanings of index in this codebase haha).
L163: if (opts?.enableRSS)
feeds are only written as files if RSS is enabled. Regardless, we still need to generate them because the feed at feedIndices.get("index")
is used as the sitemap and becomes the simplifiedIndex
later. (note that "index" is referring to index.xml
and not index.md
; in this context it means "every page on the site" not "the homepage")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I suggest that for the index, we make a separate path, since contentIndex will be populated in downstream components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I suggest that for the index, we make a separate path, since contentIndex will be populated in downstream components.
Not sure what you mean by this. what are you referring to with "index"? "index" as a key of feedIndices is going to have the feed for all pages on the site, index.xml
will be an RSS feed for the content of "index" (only emitted if enableRSS
is on), and the index contentIndex is used to generate the sitemap and populate simplifiedIndex.
"index" is part of feedIndices because that made the most sense with the structure of this file (and it means I only need one loop to generate all feeds, and it makes configuration by the user easier).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm you are right.
Suggestions added. I'm upset that the idiomatic way to concatenate in-place is to expand the second array in a |
ccbd6cc
to
a674a6e
Compare
I think the lint config is broken, it's wanting to add trailing commas to arguments of single-argument functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i actually have a really hot take on this, curious to hear your opinion @bfahrenfort ...
we should generate an RSS feed per directory UNLESS that directory has an index.md
to has noFeed: true
in the frontmatter
imo, we should do content-level config within frontmatter wherever possible but I'm curious but what you think here
I worry about build times, doing this recursively could get clunky for large gardens. The main reason I wanted to retain the initial functionality and then opt-in to my new feature is because the current type is what's expected from effectively any other RSS integration on any site/SSG. We also don't have the dynamicity to do caching like Mastodon does for their per-tag and per-user feeds. Can maybe add a plugin option for this behavior? I definitely think just adding
Can absolutely add this! Would an |
i think it shouldn't be too chunky, the processing for rss feeds is really light compared to what the rest of Quartz does so it would be dominated by the disk write speed (i.e. Hugo has this behaviour) we can also reuse nestings, e.g. if we have [content]
the rss feed for content would just be the feed of a + d |
Reuse is fascinating because that basically just needs a DFS for generation then. Can I safely do recursion here? |
you could use memoization here for the mapping of directory to rss feed and only compute if it hasnt already been computed (i think we shouldnt need recursion here) |
Ooh, any resources on that? I'll add it when I get time. |
Is this blocked? Anyway we can help? I really like these ideas:
I think this is the ideal, use top-level folders to do RSS channels |
cc/ @jackyzha0 |
Hi there, please don't ping maintainers as we all have real life and jobs to tend to. We will get to this at our own time. If you want this then feel free to contact the author to continue their works. Hopefully you would understand this |
Hi @gustavo-depaula , I think I figured out how to implement it, I'm just waiting on finding the time to do so. My idea is to build a tree of indices that matches the directory structure, and then implement |
Looks like I can implement something called a visitable tree for this. Vet my pseudocode:
I'm planning to emit feeds in the same directory as the directory it tracks, that way you can just add |
yes! this was the implication of one of my earlier reviews :) makes a lot of sense, go for it |
a9f87c0
to
86ef225
Compare
Revised implementation pseudocode:
(done) |
Will squash for cleanliness when it's ready for merge, just let me know if there are any changes I need to make on review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some tiny styling change. cc jacky for final say
// bfahrenfort: If I could finagle a Visitor pattern to cross | ||
// different datatypes (TransformVisitor<T, K>?), half of this pass could be | ||
// folded into the FeedGenerator postorder accept | ||
const detailsOf = ([tree, file]: ProcessedContent): ContentDetails => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this only concerns all folders that contains that is reachable by Quartz
so non-empty folders that doesn't have a index.md
won't have a RSS file ig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this only concerns all folders that contains that is reachable by Quartz
Yes.
so non-empty folders that doesn't have a
index.md
won't have a RSS file ig.
No, non-empty folders will all have RSS files, unless they have noRSS: true
in the frontmatter of an index.md
in the folder. detailsOf
is called once per content file in the content
array passed to emit()
.
Example:
content/
├─ afolder/
├─ bfolder/
│ ├─ a.md
│ ├─ b.md
├─ cfolder/
│ ├─ index.md
│ ├─ c.md
├─ index.md
produces:
public/
├─ bfolder/
│ ├─ a.html
│ ├─ b.html
│ ├─ index.html (default)
├─ bfolder.rss (a, b) (except any that said noRSS)
├─ cfolder/
│ ├─ index.html (user-defined)
│ ├─ c.html
├─ cfolder.rss (c, cfolder/index) (unless cfolder/index.md said noRSS)
├─ index.html
├─ index.xml (index, a, b, c, cfolder/index) (except any that said noRSS)
├─ sitemap.xml
├─ static/
│ ├─ contentIndex.json
8a0e78b
to
41d2b6e
Compare
Follow-up on #529 and accidental #865. @aarnphm